Fix infinite spinner when session endpoint is unreachable - #2636
Fix infinite spinner when session endpoint is unreachable#2636NKoech123 wants to merge 4 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
builder-io-integration
Bot
changed the title
This comment has been minimized.
This comment has been minimized.
…f05de42a5e5947ba82d5 # Conflicts: # packages/core/src/client/DefaultSpinner.tsx
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment has been minimized.
This comment has been minimized.
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Code Review Summary
PR #2636 bounds retries for the shared session endpoint, introduces an explicit unavailable state, and gives RequireSession a recoverable notice instead of redirecting or spinning forever. The core distinction between unreadable session state and a confirmed signed-out state is sound, and the retry cancellation, backoff, and focused unit coverage are good. This is a high-risk review because the change affects authentication/session handling.
Key Findings
🟡 MEDIUM — isLoading remains true after the retry budget is exhausted, which preserves compatibility for existing consumers but also permanently blocks public flows that use !sessionLoading as a gate for optional session resolution. In particular, public Clips share routes can fail to fetch their unauthenticated recording/meeting data when the session endpoint is down. Those consumers need to distinguish active loading from terminal unavailability, or the hook needs a compatibility signal that lets safe anonymous requests proceed.
🧪 Browser testing: Could not verify — the dev server was healthy, but all browser executor batches lacked browser automation tools and escalated every planned test case.
| // Callers that only read `isLoading`/`session` (most of the codebase, not | ||
| // yet migrated to `status`) must not see "unavailable" as "signed out" — | ||
| // that bounces an authenticated user through sign-in-only UI over a | ||
| // transient blip. Keeping `isLoading` true here reproduces this hook's | ||
| // pre-existing behavior for those callers (an indefinite "still resolving" | ||
| // instead of a wrong answer); only `status`-aware callers get the distinct | ||
| // "unavailable" treatment with a retry affordance. | ||
| const isLoading = status === "loading" || status === "unavailable"; |
There was a problem hiding this comment.
🟡 Unavailable sessions still permanently block public session consumers
isLoading remains true after the retry budget is exhausted. Public Clips share routes such as templates/clips/app/routes/share.$shareId.tsx and share.meeting.$meetingId.tsx gate their public data requests on !sessionLoading, so four session-endpoint failures disable those requests indefinitely even though the share data does not require a session. Update safe anonymous consumers to distinguish terminal status === "unavailable" from active loading, and add regression coverage for a public share while the session endpoint is unreachable.
Additional Info
Reported by 1 of 2 independent code-review agents; confirmed against the current PR diff and consumer paths.
3 participants
Summary
Fixes
useSessionretrying a failed session request forever while stuck in a loading state, and adds a distinctunavailablestatus with a recoverable UI inRequireSession.Problem
useSessionretried a failed/_agent-native/auth/sessionrequest every second forever while holdingisLoadingtrue. A transient 5xx, network failure, or timeout produced a spinner that never resolved and surfaced no error, stranding users indefinitely. Additionally,DefaultSpinner's stall hint always told visitors to "check the terminal running the dev server," which is meaningless on hosted/production deployments.Solution
useSessionnow caps retries at a bounded number of attempts with backoff and reports a distinctstatus: "unavailable"instead of retrying forever.RequireSessionbranches on this new status to show a recoverable notice (with "Try again" and "Reload" actions) rather than collapsing it into "signed out" (which would bounce a signed-in user to sign-in) or "loading" (which would strand them).DefaultSpinner's stall hint is also made environment-aware so it only mentions the dev server terminal in development.Key Changes
use-session.ts: addsSessionStatustype (loading|authenticated|unauthenticated|unavailable), bounded retry count (SESSION_MAX_ATTEMPTS = 4) with increasing backoff,errorandretryfields on the hook result, and keepsisLoadingtrue forunavailableso legacy consumers aren't misled into thinking the user is signed out.require-session.tsx:ResolvedSessionGatenow readsstatus/retryfromuseSession, only redirects onunauthenticated, and renders a newSessionUnavailableNoticecomponent (with "Try again" and "Reload page" buttons) whenstatus === "unavailable".DefaultSpinner.tsx: adds astallHint()helper that returns dev-specific wording whenimport.meta.envindicates development, and generic "try reloading" wording otherwise (including when the Vite env is absent, e.g. in prebuilt dist consumption).app-providers.spec.tsx,require-session.spec.tsx, anduse-session.spec.tsxto cover the newstatusfield and the "unavailable" retry/notice behavior.To clone this PR locally use the Github CLI with command
gh pr checkout 2636You can tag me at @BuilderIO for anything you want me to fix or change